feat: implement integer type to comply with spec #98
Conversation
The JSONPath spec states that integers, e.g., those used as indices or in slice selectors, must be within a specific range [-2^53 +1, 2^53 - 1]. This adds a new Integer type to be used to represent such integers, and check that they stay within that range. It also caught a couple of places where overflows would have led to panics.
Integer initialization is done for small safe values, e.g., 0, 1, -1, so this uses a panicing version of the from_i64_opt function to remove the need for unwrap everywhere.
| /// | ||
| /// This will panic if the inputted value is out of the valid range | ||
| /// [-(2<sup>53</sup>)+1, (2<sup>53</sup>)-1]). | ||
| pub fn from_i64_unchecked(value: i64) -> Self { |
There was a problem hiding this comment.
Would it make sense to additionally offer conversion functions (or From) for i32? i32 is probably somewhat common and can always be converted to Integer without an error.
Maybe also for u32?
There was a problem hiding this comment.
Certainly, I have no issues with extending the API. Since it is public, as a result of being in core, then doing so would be useful.
| /// Take the absolute value, producing `None` if the resulting value is outside | ||
| /// the valid range [-(2<sup>53</sup>)+1, (2<sup>53</sup>)-1]). | ||
| pub fn checked_abs(mut self) -> Option<Self> { | ||
| self.0 = self.0.checked_abs()?; | ||
| self.check().then_some(self) | ||
| } |
There was a problem hiding this comment.
Can this actually ever return None? The Integer limits don't permit i64::MIN so I think self.0.checked_abs() always returns Some.
And for the value range of Integer MIN.abs() == MAX, so self.check() should always be true.
There was a problem hiding this comment.
This is a great point. I think this can change to just return Self
| pub fn checked_add(mut self, rhs: Self) -> Option<Self> { | ||
| self.0 = self.0.checked_add(rhs.0)?; | ||
| self.check().then_some(self) | ||
| } |
There was a problem hiding this comment.
Out of curiosity (applies to all the mutating methods): Is this pattern of both mutating self and returning self that common (when the struct is not a 'builder')? It might be a bit error-prone, e.g. users might not expect (or notice) that self is mutated. Should instead a new Integer be returned, and self not be mutated?
Also, I am not sure if this approach of first doing self.0 = ... and then calling self.check() is safe. If the caller ignores the result, or continues to use the Integer afterwards, it now contains an invalid value.
There was a problem hiding this comment.
Yeah, this is definitely a foot gun. I don't think it is a problem from where Integer is used in serde_json_path, but I will definitely change this.
Since these changes are not released yet I can make breaking changes to the API of Integer.
There was a problem hiding this comment.
Ah sorry, my comment was wrong. Since it is taking mut self and not &mut self it was actually behaving as expected since Integer is Copy, and actually ended up creating a copy. But the code might still be a bit confusing then.
|
@Marcono1234 - thanks for the comments and for calling out the quirks in the API - I will submit another PR to update the API, and may request your review. |
This PR is to address the recent CTS failure.
The JSONPath spec states that integers, e.g., those used as indices or in slice selectors, must be within a specific range [-2^53 +1, 2^53 - 1] (see here).
The CTS recently added tests to verify this behaviour in jsonpath-standard/jsonpath-compliance-test-suite#90, and
serde_json_pathwhich was not accounting for it, was failing the updated CTS.This PR adds a new
Integertype to represent such integers and check that they stay within that range. It also caught a couple of places where overflows would have led to panics.